fix(desktop): return expected failures as codes across IPC - #4878
Conversation
3af86a4 to
8aaadc1
Compare
b98a8d3 to
ba1495e
Compare
7c5f3fd to
b84d0d6
Compare
b84d0d6 to
b05a71d
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for moving expected failures into structured results. The overall direction looks sound: Runtime/Host keeps ownership of configuration and Plan state, while Desktop transports the failure code and the renderer owns its presentation. I found one remaining serialization boundary issue in the Plan-abandon path, noted inline.
For simplification, I would keep attachment rejection presentation separate from showSkillInvocationFeedback: its new attachment branch makes a Skill-specific helper responsible for a general submit failure. This is an optional cleanup, not a correctness blocker.
Reviewed at b05a71dde11606456ef1ff902b7dacfc81889d5f. The inline finding was checked with a minimal Electron 43.4.1 contextBridge probe; the full Desktop suite was not rerun, and CI is still running.
AI assistance: Codex with a deep reviewer; the finding was cross-checked against the production path and the Electron probe.
b05a71d to
2dc82e4
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the fix. An independent fresh-eye re-review of 2dc82e4596b77ed8cbe42718ce5454c792cd79d7 found no remaining P0–P3 code issue.
The previous Plan-abandon serialization defect is fixed: the structured result remains intact through main and preload, and the renderer-owned session-settings adapter unwraps it after the contextBridge boundary. The setting-update and attachment-rejection paths were also traced through their production consumers. I do not have a further simplification to request.
Two verification items remain: the PR currently conflicts with main, and the required test check failed on the upward-traversal-holds-turn-geometry Storybook render timeout; Windows packaging passed. That story was not changed directly, but this PR does touch AppShell, so the single timeout does not establish that the failure is unrelated. Please resolve the conflicts and verify the resulting head, including a focused Electron check of busy-setting, Plan-abandon, and attachment-rejection feedback.
This pass used exact-source checks and the live CI evidence; it did not rerun the full build or packaged Electron E2E.
AI assistance: a new Codex deep reviewer performed the fresh-eye pass, followed by a primary-agent cross-check.
ae40c34 to
fe92570
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the update! The Session-setting and Plan-abandon paths look sound in this pass. I found one remaining WorkHub attachment issue, noted inline.
This also changes user-visible feedback. Could you add actual before/after screenshots of the resulting toasts? The current text examples help explain the wording, but do not show the interaction. A short recording covering busy-setting rejection, a Plan-abandon conflict, and attachment rejection would be helpful too.
I checked head fe9257020: 106 focused tests passed, and the Electron boundary probe confirmed both the corrected paths and the remaining WorkHub issue. This does not replace full UI acceptance.
AI assistance: Codex and Reviewer Sol, with the finding independently reproduced.
d76a809 to
5092dbe
Compare
5092dbe to
2df6502
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for closing the WorkHub attachment gap and adding real screenshots. I found no remaining P0–P3 issue at 2df6502. Expected codes now survive main, preload and contextBridge, with typed errors reconstructed in the renderer adapter. The shared attachment preparation also removes duplication. Verification covered 44 focused tests and an isolated Electron probe using this head’s full preload, real WorkHub handler and renderer adapters; setting, Plan and WorkHub attachment failures retained their codes and localized feedback. CI is green. The current conflict in create-workhub-services.ts still needs resolution: keep both main’s new interaction ports and this PR’s attachment result unwrapping, then verify the updated head before merging. AI-assisted review with Codex and Reviewer Sol.
Expected failures must survive both Electron IPC and contextBridge serialization so the renderer can present actionable localized copy. Return structured envelopes through those boundaries, unwrap them only in renderer-owned adapters, and keep attachment rejection presentation separate from Skill feedback. Runtime configuration guards use typed transition errors instead of localized prose. Generated-by: Claude Code Generated-by: OpenCode Generated-by: Codex
2df6502 to
0bda47e
Compare
…f0a5d) Eight upstream commits. One reaches the new renderer's contracts: apache#4878 returns expected failures as codes across IPC — the five Session setters (`setPermissionMode`, `setCollaborationMode`, `setOrchestrationMode`, `setModelConfiguration`, `setThinkingLevel`) answer `DesktopSessionUpdateResult` instead of throwing, `abandonPlanProposal` answers `PlanControlIpcResult` like its siblings, `send` / `submitMessage` gain an `attachment_blocked` refusal, and the attachment guard throws a typed `AttachmentIngestBlockedError` in place of the `attachment_ingest:<code>` message token. Also in: apache#5216 makes HTML artifacts directly openable (`app.showArtifactInFolder` beside `openArtifactPath`, which now hands an HTML artifact to the default app; `isArtifactUserVisible` admits HTML tool results), apache#5198 unifies WorkHub conversation identity and Host-owned choices (an Astryx `ChoicePanel` in packages/ui, `keyboardHint` copy, a `preserveFocus` scroll target), apache#5249's skill picker fix (`selectedSkillIds` in `chat-input-behavior`), apache#4815 admits structured-only Messages (`hasMeaningfulMessageContent` in core), apache#4862's ACP live session lifecycle in the CLI, apache#5204's workbar tab scrollbar css and apache#5180's wider locale hygiene gate. Resolution per the sync policy: conflicts under the old renderer's trees, packages/ui's deleted components, stories, e2e specs and the main tests that import them stay deleted, and upstream's new files there are dropped (`features/workhub/model/workspace-name.ts`, packages/ui's `choice-panel.tsx` and its `index.ts` export, the `styles/base.css` / `workhub.css` / `maka-tokens.css` edits, `expected-error-presentation.test.ts` and the WorkHub main tests). git's rename pairing had put upstream's `features/session-settings/ports.ts`, `features/workhub/testing.ts` and `platform/desktop/create-session-settings-services.ts` into `bridge/e2e-fixture.ts`, `components/ui/skeleton.tsx` and `lib/ported/display-frame-scheduler.ts`; all three keep ours. The renderer architecture ledger keeps ours, rewritten with `--write`. The e2e budget and `transcript-scroll-cost.spec.ts` keep ours; upstream's new `expected-failure-feedback.spec.ts` is trimmed to its second case (the IPC round trip of the setting and Plan codes), the first needing the WorkHub surface this build does not ship. Re-implemented for the new contracts: - `bridge/sessions.ts` unwraps every update result and rethrows a refusal as `ExpectedOperationError` (new `bridge/expected-operation-error.ts`, ported from upstream's `operation-diagnostics.ts`), so the turn actions store and every caller keep awaiting a summary. `localizedShellErrorMessage` renders the code through the new `updateFailures` copy on every surface and `AttachmentIngestBlockedError` by its `code`; `sessionSettingFailureCopy` is upstream's. - `ChatInput` routes an `attachment_blocked` refusal through upstream's `showSubmissionFeedback` and keeps the draft with the ingest reason; `showSkillInvocationFeedback` stays exported for the partial-success toast. - `FilesTab` reveals through `showArtifactInFolder`; an HTML row shows "View in Maka", its menu offers "Open in Default App" first, and its preview's external action opens rather than reveals. `artifact-copy` gains `viewInMaka` / `openInDefaultApp` in three locales. - `TipTapEditor` hides Skills already in the draft from the picker (apache#5249); the chips are atoms here, so the set is read from the document rather than from the `/skill:x` text upstream scans. - `composer-state.test.ts` asserts the typed preflight error. packages/ui: `use-chat-scroll.ts` merged cleanly (`preserveFocus`) on top of our `holdTurn` extensions; `conversation-copy.ts` and `chat-input-behavior.ts` take upstream's additions. apache#5217's live-turn buffer stays out as before. The compatible-change declaration is unchanged this round. The release checklist's baseline note records that the eight commits add no unlisted renderer surface. Gates: build:test + build:renderer, typecheck, biome lint and format, locale hygiene (the widened apache#5180 gate), ASF headers, renderer architecture ledger (rewritten with `--write`), e2e budget, third-party notices, knip (39 unused files, unchanged from the twelfth sync), workspace dist tests (desktop 3484 of 3498 with 14 skipped, every other workspace green), Electron smoke (44 checks, no renderer errors), core-dialogue smoke, streaming-switch smoke, and the trimmed `expected-failure-feedback` e2e case against the real preload. `packages/runtime` `model-adapter-onerror` fails on this machine before and after, as in the eleventh and twelfth syncs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Summary
Changing a busy task's permission, plan, orchestration, or model setting shows a generic fallback instead of the actionable reason. WorkHub attachment validation can likewise collapse to “Retry” because Electron discards custom
Errorfields at IPC boundaries.Expected failures now cross Electron as discriminated return values while unexpected failures still throw. Session settings and Plan Mode preserve Runtime Host codes, and attachment send paths preserve
AttachmentIngestBlockedError.code; renderer adapters reconstruct typed errors for the locale presenters. No product path parsesError.message.Boundaries: the Runtime Host protocol is unchanged, so there is no Host version skew. The plan-mode write still uses the session-settings port to keep the renderer architecture ratchet flat.
Refs #2672
Verification
WorkHub attachment rejection with a specific reason: 0/1 before → 1/1 after.
Before
After
A
session_busyresult also reaches both locale presenters as structured data:Not run: packaged build.
AI use
Select exactly one:
Tool(s) and scope: Claude Code and Codex — traced the failure paths, implemented the structured envelopes and locale presentation, simplified the error contract and comments, added tests and Electron evidence, rebased onto current main, and updated this description. The protocol boundary and user-facing copy were reviewed by hand.
Checklist
Does this PR entail a change in behavior?